Fix: VS Code themes discard their own terminal.ansi* palette - #36
Open
pullapprove5[bot] wants to merge 1 commit into
Open
Fix: VS Code themes discard their own terminal.ansi* palette#36pullapprove5[bot] wants to merge 1 commit into
pullapprove5[bot] wants to merge 1 commit into
Conversation
A theme resolved from VS Code JSON dropped its `terminal.ansi*` colors on the floor: `COLOR_MAP` never read them, so `ansiPaletteFor` found no entry for the `vscode-*` id and substituted the generic Tomorrow Night / GitHub Light ramp. Turning on "match VS Code theme" with Catppuccin therefore gave Catppuccin chrome around another theme's terminal output — the two-themes- in-one-window failure `terminal-palettes.ts` exists to prevent for the bundled themes. `UiTheme` now has an optional `ansi` ramp; `resolveVscodeTheme` fills it from the sixteen keys VS Code themes publish, all-or-nothing so a partial ramp can't pair one theme's normals with another's brights. The generic ramp stays as the fallback it was meant to be. Getting the ramp to the terminal needs the theme object, not just its id: a VS Code-derived theme is built on the fly and is not in `getAllUiThemes()`. `applyUiTheme` now records the applied theme in `lib/active-theme.ts` and `buildXtermTheme` reads it there, which also retires the `data-ui-theme` round-trip that existed only to rejoin a palette to its theme.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What was wrong
COLOR_MAPindesktop/ui/lib/vscode-theme-resolver.tshad no entries forterminal.ansiBlack…terminal.ansiBrightWhite, so a theme resolved from VS Code JSON discarded its own 16-color terminal ramp.ansiPaletteForthen found noPALETTESentry for the generatedvscode-*id and substituted the generic Tomorrow Night / GitHub Light ramp. With "match VS Code theme" on, a Catppuccin user got Catppuccin chrome around another theme's terminal output — exactly the two-themes-in-one-window failureterminal-palettes.tsexists to prevent for bundled themes.What changed
UiThemegained an optionalansi?: AnsiPalette(lib/ui-themes.ts). TheAnsiPaletteinterface moved there fromcomponents/Terminal/terminal-palettes.ts, since it is now part of a theme's shape; the values and their provenance stay interminal-palettes.ts, which imports the type.resolveVscodeThemepopulates it from the sixteenterminal.ansi*keys, all-or-nothing: a partially published ramp topped up from the shared fallback would pair one theme's normals with another's brights, which is the same mismatch in miniature rather than a fix for it.ansiPaletteFornow takes the theme, not an id, and preferstheme.ansi→ the transcribedPALETTES[theme.id]→ the shared ramp. The existing light/dark legibility nudge is unchanged and applies to whichever ramp wins.getAllUiThemes(), so an id alone can't find it again.applyUiThemerecords the applied theme in a new one-purpose modulelib/active-theme.ts, andbuildXtermThemereads it from there. That retires theel.dataset.uiThemeround-trip, which existed only to rejoin a palette to its theme and now has no readers. The module is separate fromui-themes.tsso the terminal doesn't pull in the theme catalog (and Shiki behind it).I did not do the other half of the finding's optional suggestion (making
PALETTESa requiredUiThemefield for bundled themes) — that's a larger reshuffle of the bundled theme table and isn't needed to fix the reported defect.Tests
components/Terminal/xterm-theme.test.tsupdated to the new plumbing (it drove the terminal throughdataset.uiTheme), plus two new cases: a ramp carried on the active theme beats the shared fallback, and a null active theme still gets the shared ramp. Existing assertions — bundled ramps, bright≠normal, the fallback for a theme with no palette — are unchanged.lib/vscode-theme-resolver.test.ts: a detection carrying all sixteen keys yields the exact ramp; one with none, and one missing a single key, both leaveansiunset.What I could not verify
The repo's tooling is not runnable in this sandbox and I did not run the tests.
desktop/node_modulesis absent and there is no network (npm ci --offlinefails on an uncached tarball; the registry is unreachable), soscripts/test(tsc --noEmit+vitest) andprettier --checkcould not run. What I did check: every changed and added file parses cleanly undernode --experimental-strip-types --check, and I hand-checked formatting against Prettier's 80-column defaults and the surrounding code. The type-level changes (the newansiPaletteForsignature at its two call sites, the movedAnsiPaletteimport) are mechanical, but they are unverified by a compiler — worth ascripts/testrun before merge.Opened by a PullApprove implementation run (implement-finding v1) for the finding PA-27 — VS Code themes discard their own terminal.ansi palette*.
Merging this is what closes the finding as fixed.